-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for more complicated documentation examples #8287
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, thank you for tackling this !
@@ -0,0 +1,9 @@ | |||
from .bad2 import count_to_two # [cyclic-import] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we take into account the fact that if we want to do multiple examples properly displayed we're going to want to have bad.py, bad2.py, bad3.py (...) too ? Maybe the display can change if there's no good2.py, (...) matching the bad2.py (...) ? Edit: Did not notice it's in a directory, I think it's compatible.
doc/test_messages_documentation.py
Outdated
if (message_dir / "bad.py").exists(): | ||
suite.append( | ||
(message_dir.stem, message_dir / "bad.py"), | ||
) | ||
elif (message_dir / "bad").is_dir(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haa, this make sense, we can have directories too for multiple example. (bad / bad2/...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in that case it makes more sense to just use one file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also do bad.py, neutral.py, lawful.py, chaotic.py and good.py
pylint/checkers/imports.py
Outdated
if imported_module is None: | ||
|
||
# If we can't find the import we still try and build the import graph with our | ||
# best guess: the relative name of the import. | ||
imported_module_name = imported_module.name if imported_module else node.modname | ||
if imported_module_name is None: | ||
return | ||
|
||
for name, _ in node.names: | ||
if name != "*": | ||
self._add_imported_module(node, f"{imported_module.name}.{name}") | ||
self._add_imported_module(node, f"{imported_module_name}.{name}") | ||
else: | ||
self._add_imported_module(node, imported_module.name) | ||
self._add_imported_module(node, imported_module_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like something that could be it's own PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah and it is breaking tests... Without it this test is very flaky so I'm not sure what to do
f46f18b
to
7227c07
Compare
@Pierre-Sassoulas I kept in the I think this PR also closes #7143 right? |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8287 +/- ##
==========================================
- Coverage 95.46% 95.46% -0.01%
==========================================
Files 177 177
Lines 18704 18702 -2
==========================================
- Hits 17856 17854 -2
Misses 848 848
|
Yes, amazing ! |
doc/exts/pylint_messages.py
Outdated
) | ||
elif (data_path / "bad").exists(): | ||
bad_one = f"""\ | ||
File one: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of File one
/ File two
, we should have the actual name of the file (bad.py::
. This would also simplify the handling code that can become generic and handle as many file as we want. We can also name it however we want (it's the directory prefix that indicate badness / goodness).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this suggestion! Will (hopefully) work on this tomorrow!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you agree with the good/ multiple fle and I'll do it :)
doc/exts/pylint_messages.py
Outdated
if bad_py_path.exists(): | ||
bad_code = _get_titled_rst( | ||
title="Problematic code", text=_get_python_code_as_rst(bad_py_path) | ||
) | ||
elif (data_path / "bad").exists(): | ||
bad_files: list[str] = [] | ||
for file in (data_path / "bad").iterdir(): | ||
if file.suffix == ".py": | ||
bad_files.append( | ||
f"""\ | ||
``{file.name}``: | ||
|
||
.. literalinclude:: /{file.relative_to(Path.cwd())} | ||
:language: python | ||
|
||
""" | ||
) | ||
bad_code = _get_titled_rst(title="Problematic code", text="".join(bad_files)) | ||
else: | ||
bad_code = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should create a generic function for this as we also need to handle good/
directories with multiple file imo. I can do it, it's a lot easier with pycharm than vscode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I didn't want to add code that we don't actively use right now, but go ahead and add it! Whatever makes it easier to complete this process the quickest!
doc/exts/pylint_messages.py
Outdated
) | ||
elif (data_path / "bad").exists(): | ||
bad_files: list[str] = [] | ||
for file in (data_path / "bad").iterdir(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to sort here, so bad.py is before bad2.py
for file in (data_path / "bad").iterdir(): | |
for file in sorted((data_path / "bad").iterdir()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turn out it's not that easy to sort Path π took me embarrassingly long to understand
I have commits almost ready that clean up this PR. Hopefully can finish this tomorrow. |
99ef71b
to
d2d50d9
Compare
Not clearing that reporter after every file shows some underlying issues that we never seen. Nice to fix these as well I think π |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but it's impossible to check in read the doc now that the cyclic-import is removed.
def on_set_current_module(self, module: str, filepath: str | None) -> None: | ||
self.messages = [] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice !
Yeah, like I said; the If you know of another message where you want multiple files and that isn't flakey I can add it as well. Otherwise I think we should just merge this. I did test this locally with multiple files and checked that the tests still worked (which is how I found the issues with the reporter). |
Maybe too many lines ? It's a little insulting to readers though. Otherwise, duplicated code ? I can add this and test locally myself :) |
Duplicated code would be a good one! Perhaps make a PR that has this branch as target branch? |
Co-authored-by: Pierre Sassoulas <[email protected]>
9453e4c
to
bbfd103
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let you check the example, and then let's merge. The result is pretty good, excited to make this live ! π
Both https://pylint--8287.org.readthedocs.build/en/8287/user_guide/messages/warning/while-used.html and https://pylint--8287.org.readthedocs.build/en/8287/user_guide/messages/refactor/duplicate-code.html are definite improvements! Indeed happy we have this now! |
btw - is the |
I think it might make sense indeed! |
In pylint-dev#7162, "we decreed" that the `bad_code` goes first, then the `good_code`. Fixup current changes, to revert "back to standards". Additionally, tackle the comment here pylint-dev#8287 (comment) "Message emitted" is technically still code (`%-formatting`); let us make it stand out properly. Signed-off-by: Stavros Ntentos <[email protected]>
In #7162, "we decreed" that the `bad_code` goes first, then the `good_code`. Fixup current changes, to revert "back to standards". Additionally, tackle the comment here #8287 (comment) "Message emitted" is technically still code (`%-formatting`); let us make it stand out properly. Signed-off-by: Stavros Ntentos <[email protected]>
Type of Changes
Description
Refs #7897
Closes #7143